Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[J-Turtle] Fix uninitialized values in NavSatFix and add missing NavSatStatus UNKNOWN #220

Merged
merged 4 commits into from
Apr 2, 2024

Conversation

Ryanf55
Copy link
Contributor

@Ryanf55 Ryanf55 commented Apr 4, 2023

I would love to get this in IRON by the freeze on Mon. April 17, 2023

* Fixes ros2#196

Signed-off-by: Ryan Friedman <ryanfriedman5410+github@gmail.com>
@Ryanf55 Ryanf55 requested a review from tfoote as a code owner April 4, 2023 07:02
@Ryanf55 Ryanf55 changed the title Fix unitialized values in NavSatFix and add missing UNKNOWN [IRON] Fix unitialized values in NavSatFix and add missing UNKNOWN Apr 4, 2023
Copy link
Contributor

@tfoote tfoote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a few syntax suggestions but overall I think that this change makes sense. Could you do a few fixups and we can circulate this further. It would be great to get some input from other GPS users/maintainers. And also to look at where in the public codebase this is used to make sure that there doesn't seem to be any major incompatibilities/assumptions about this value that we'll break.

sensor_msgs/msg/NavSatFix.msg Outdated Show resolved Hide resolved
sensor_msgs/msg/NavSatStatus.msg Outdated Show resolved Hide resolved
@@ -4,12 +4,13 @@
# type and the last time differential corrections were received. A
# fix is valid when status >= STATUS_FIX.

int8 STATUS_UNKNOWN = -2 # status is not yet set
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense to me to have an uninitialized state as the default.

Copy link
Contributor Author

@Ryanf55 Ryanf55 Feb 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I view it as a safety problem that the default state is "healthy". Because ROSIDL supports a default constructor in C++, users of NavSatFix are not required to fill this out. Thus, if they take shortcuts, only fit out the lat-long fields, and lose GPS signal,they now report the status as having a fix even though the data is invalid and might not realize it.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't defaulting to STATUS_NO_FIX fix the problem?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also default the latitude and longitude to NAN to avoid accidental visits to Null Island. https://en.wikipedia.org/wiki/Null_Island

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't defaulting to STATUS_NO_FIX fix the problem?

Yes, but adding a new value makes it exactly clear between these two situations:

  1. The ROS driver has started but it has not received a fix status message from the GPS =>STATUS_UNKNOWN
  2. The ROS driver has communicated with the GPS, and the GPS said it has no fix => STATUS_NO_FIX

I'd vote for keeping these as distinct as one means you might have a wiring or baud issue, and the other means that you just need to wait for it to warm up or get a better view of the sky.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a fan of setting the latitude and longitude to NAN as default (assuming the ROS IDL lets us do that).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also default the latitude and longitude to NAN to avoid accidental visits to Null Island. https://en.wikipedia.org/wiki/Null_Island

This is not currently possible. See my docs PR which explains what's currently possible with NaN.
ros2/ros2_documentation#4210

I am working on adding support for NaN defaults and NaN constants, but it's not clear whether it will make the Jazzy release.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding in the NaN logic is reasonable, but I think that should be a sanity check of the data, more than the canonical way to know what the status is. Checking all field values for NaN can be very expensive versus being able to quickly check an enumeration and stop processing data early.

Copy link
Contributor Author

@Ryanf55 Ryanf55 Mar 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be more of a NaN fan if tooling supported it better, but there's currently no way to to set a NaN value in DDS IDL, nor is it supported in JSON. See here for implementation notes. I'd prefer not hold up this PR which was intended as a bugfix with other things like NaN support.
ros2/rosidl#789

@tfoote
Copy link
Contributor

tfoote commented Apr 4, 2023

For reference default value syntax is defined here: https://design.ros2.org/articles/legacy_interface_definition.html

* Don't over declare ones that are already unknown-initialized

Signed-off-by: Ryan Friedman <ryanfriedman5410+github@gmail.com>
Signed-off-by: Ryan Friedman <ryanfriedman5410+github@gmail.com>
@Ryanf55 Ryanf55 requested a review from tfoote April 4, 2023 22:53
@Ryanf55
Copy link
Contributor Author

Ryanf55 commented Apr 4, 2023

All fix-ups are complete.
The addition of unknown would make sense here: https://github.com/ros-drivers/nmea_navsat_driver/blob/b168f499349e5f26e3aed9982deb44ed2f41f8e2/src/libnmea_navsat_driver/driver.py#L110

When you are happy, please share/tag some stakeholders.

Cheers!

@tfoote
Copy link
Contributor

tfoote commented Apr 10, 2023

@evenator @mikepurvis @danthony06 @kmhallen @hortovanyi

I've seen your info on GPS packages. Do you have any concerns?

Copy link
Contributor

@tfoote tfoote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good to me. I've reached out for others thoughts.

@danthony06
Copy link

@tfoote I think the changes look good, and mirror some experimental changes I've made for the gps_common package in ROS 2.

One thing we've seen, and have gotten some open issues with, on the gps_common package is better real time kinematic GPS support. If we're making changes to this message, is it worth thinking about adding some additional definitions to indicate it's an RTK capable system, and whether or not it's actively using RTK corrections?

@hortovanyi
Copy link

If the device is uninitialised it shouldn't be publishing a NavSatFix message IMHO.

NavSatFix is based on the older NMEA standard ..

Will be releasing an update soon that will publish NavSatFix message from the UBX data, as a separate node with a high precision location.

The UBLOX UBX values are different to the older NavSatFix values https://github.com/aussierobots/ublox_dgnss/blob/main/ublox_ubx_msgs/msg/UBXNavStatus.msg
https://github.com/aussierobots/ublox_dgnss/blob/main/ublox_ubx_msgs/msg/GpsFix.msg
https://github.com/aussierobots/ublox_dgnss/blob/main/ublox_ubx_msgs/msg/CarrSoln.msg

Of most concern to us is if the differential correction has been applied such that high precision (ie centimetre level accuracy) location is possible

@hortovanyi
Copy link

Heres a link to the proposed code for translation the status message for NavSatFix from UBX data. Havent merged and published this branch yet https://github.com/aussierobots/ublox_dgnss/blob/1e10c8562ad79a8fca1540a9c78968c117920980/ublox_nav_sat_fix_hp_node/src/ublox_nav_sat_fix_hp_node.cpp#L174

@danthony06
Copy link

I don't think we're necessarily tied to NMEA 0183, and aren't directly representing that information in this message. There's plenty of non-NMEA 0183 based GPS receivers that can indicate things like a hardware fault that could be encapsulated by this unknown status that are separate from a normally functioning GPS receiver that just hasn't acquired a fix yet, and we still send out a NavSatFix message for them.

@hortovanyi
Copy link

@danthony06 in that scenario from a UBX perspective there is no fix https://github.com/aussierobots/ublox_dgnss/blob/main/ublox_ubx_msgs/msg/GpsFix.msg
ublox_ubx_msgs::msg::GpsFix::GPS_NO_FIX

which translate through to
sensor_msgs::msg::NavSatStatus::STATUS_NO_FIX

@Ryanf55
Copy link
Contributor Author

Ryanf55 commented Apr 10, 2023

I don't think we're necessarily tied to NMEA 0183, and aren't directly representing that information in this message. There's plenty of non-NMEA 0183 based GPS receivers that can indicate things like a hardware fault that could be encapsulated by this unknown status that are separate from a normally functioning GPS receiver that just hasn't acquired a fix yet, and we still send out a NavSatFix message for them.

I've used this message with many non-NMEA message standards. If there was a new ROS message for every GPS protocol, it would be unusable and couple two interfaces together.

@Ryanf55
Copy link
Contributor Author

Ryanf55 commented Apr 10, 2023

If the device is uninitialised it shouldn't be publishing a NavSatFix message IMHO.

NavSatFix is based on the older NMEA standard ..

Will be releasing an update soon that will publish NavSatFix message from the UBX data, as a separate node with a high precision location.

The UBLOX UBX values are different to the older NavSatFix values https://github.com/aussierobots/ublox_dgnss/blob/main/ublox_ubx_msgs/msg/UBXNavStatus.msg https://github.com/aussierobots/ublox_dgnss/blob/main/ublox_ubx_msgs/msg/GpsFix.msg https://github.com/aussierobots/ublox_dgnss/blob/main/ublox_ubx_msgs/msg/CarrSoln.msg

Of most concern to us is if the differential correction has been applied such that high precision (ie centimetre level accuracy) location is possible

What is the effect of a subscribing application knowing it's RTK capable? We just populate the covariance and RTk happens to have a really low covariance when it goes into the state estimation.

@danthony06
Copy link

I don't think we're necessarily tied to NMEA 0183, and aren't directly representing that information in this message. There's plenty of non-NMEA 0183 based GPS receivers that can indicate things like a hardware fault that could be encapsulated by this unknown status that are separate from a normally functioning GPS receiver that just hasn't acquired a fix yet, and we still send out a NavSatFix message for them.

I've used this message with many non-NMEA message standards. If there was a new ROS message for every GPS protocol, it would be unusable and couple two interfaces together.

Yes, we also do this with the open source Novatel GPS driver to publish NavSatFix messages. I don't think we're disagreeing here.

The benefit of knowing if the receiver is RTK receiver is it helps remove ambiguity if the receiver is not functioning properly, or is in degraded conditions. I'm specifically thinking of times where I've been trying to diagnose a faulty RTK receiver in the field, and it's nice to know if the system believes it's applying RTK corrections and the fix is still bad, or if it's RTK capable, but is not using them for some other reason. Looking at the covariances doesn't necessarily give me that level of insight into the system.

@hortovanyi
Copy link

hortovanyi commented Apr 10, 2023 via email

@Ryanf55
Copy link
Contributor Author

Ryanf55 commented Apr 10, 2023

I don't think we're necessarily tied to NMEA 0183, and aren't directly representing that information in this message. There's plenty of non-NMEA 0183 based GPS receivers that can indicate things like a hardware fault that could be encapsulated by this unknown status that are separate from a normally functioning GPS receiver that just hasn't acquired a fix yet, and we still send out a NavSatFix message for them.

I've used this message with many non-NMEA message standards. If there was a new ROS message for every GPS protocol, it would be unusable and couple two interfaces together.

Yes, we also do this with the open source Novatel GPS driver to publish NavSatFix messages. I don't think we're disagreeing here.

The benefit of knowing if the receiver is RTK receiver is it helps remove ambiguity if the receiver is not functioning properly, or is in degraded conditions. I'm specifically thinking of times where I've been trying to diagnose a faulty RTK receiver in the field, and it's nice to know if the system believes it's applying RTK corrections and the fix is still bad, or if it's RTK capable, but is not using them for some other reason. Looking at the covariances doesn't necessarily give me that level of insight into the system.

Yes, I was agreeing. Happy to add that fix type, a STATUS_RTK_FIX

@Ryanf55
Copy link
Contributor Author

Ryanf55 commented Apr 10, 2023

The main reason to publish the NavSatFix message is for compatibility with existing tools and visualisation of location with co-variance matrices. It really lacks a lot of other detail. Such as which constellations have been used, spoofing, dead reckoning, power state to name a few. If there was a hardware issue wouldn’t it be better to publish a diagnostic message? https://github.com/ros/diagnostics/blob/ros2/README.md diagnostics/README.md at ros2 · ros/diagnostics github.com

On 11 Apr 2023, at 7:15 am, David Anthony @.***> wrote: I don't think we're necessarily tied to NMEA 0183, and aren't directly representing that information in this message. There's plenty of non-NMEA 0183 based GPS receivers that can indicate things like a hardware fault that could be encapsulated by this unknown status that are separate from a normally functioning GPS receiver that just hasn't acquired a fix yet, and we still send out a NavSatFix message for them. I've used this message with many non-NMEA message standards. If there was a new ROS message for every GPS protocol, it would be unusable and couple two interfaces together. Yes, we also do this with the open source Novatel GPS driver to publish NavSatFix messages. I don't think we're disagreeing here. The benefit of knowing if the receiver is RTK receiver is it helps remove ambiguity if the receiver is not functioning properly, or is in degraded conditions. I'm specifically thinking of times where I've been trying to diagnose a faulty RTK receiver in the field, and it's nice to know if the system believes it's applying RTK corrections and the fix is still bad, or if it's RTK capable, but is not using them for some other reason. Looking at the covariances doesn't necessarily give me that level of insight into the system. — Reply to this email directly, view it on GitHub <#220 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAVTAVBJ77472ILIZ74PITDXARZ7NANCNFSM6AAAAAAWSIZ4WY. You are receiving this because you were mentioned.

Agree, this is approach I've used too to use the diagnostics topic, for GPS diagnostics.

The PR here is scoped for fixing the missing enumerations. Given the timeline for Iron freeze, I'd like to keep this discussion focused on small changes here to those specific enums, rather than adding in N number of other fields that may push back the merge beyond Iron. For Iron users, until then, diagnostics can be useful, and they can always use the GPSFix message in gps_common which has a lot more detail.

I'm happy to work with the stakeholders to define the GPS message to support all the kinematics information.. but I don't think it needs to be done in this PR.

@hortovanyi
Copy link

This is what the UBX nav status message has in it

navigation fix status information

bool diff_corr # differential corrections available
bool carr_soln_valid # valid carrSoln

It is separate from the gps fix value

@danthony06
Copy link

@Ryanf55 sounds good to me. Sorry to derail the conversation about the RTK status.

@hortovanyi
Copy link

my last comment - STATUS_UNKNOWN is STATUS_NO_FIX. There is no need to differentiate. If the device is not initialised or in fault or spoofed (compromised) it shouldn't publish a NavSatFix message

@Ryanf55
Copy link
Contributor Author

Ryanf55 commented Apr 10, 2023

This is what the UBX nav status message has in it

navigation fix status information

bool diff_corr # differential corrections available bool carr_soln_valid # valid carrSoln

It is separate from the gps fix value

Can you provide the reference?

If the device is not initialised or in fault or spoofed (compromised) it shouldn't publish a NavSatFix message

This puts the burden on the subscribing application to rate-check the NavSatFix messages, and then handle timeout appropriately. Currently, in Foxglove map panel for example, it colors the icon based on the message. If the position fails, and it stops publishing, their UI will just show the last message received, but it just looks like the vehicle has stopped moving. When you publish status known, it colors the icon red.
I do not agree with your assertion there; that's not how it's used in all cases. If you think the above use case is abusing the message, then perhaps you can refer to some documentation on usage that would disagree with Foxglove's implementation?

@hortovanyi
Copy link

This is what the UBX nav status message has in it

navigation fix status information

bool diff_corr # differential corrections available bool carr_soln_valid # valid carrSoln
It is separate from the gps fix value

Can you provide the reference?
Pages 155 and 156 3.15.20 UBX-NAV-STATUS from https://content.u-blox.com/sites/default/files/documents/u-blox-F9-HPG-1.32_InterfaceDescription_UBX-22008968.pdf
https://content.u-blox.com/sites/default/files/documents/u-blox-F9-HPG-1.32_InterfaceDescription_UBX-22008968.pdf

If the device is not initialised or in fault or spoofed (compromised) it shouldn't publish a NavSatFix message

This puts the burden on the subscribing application to rate-check the NavSatFix messages, and then handle timeout appropriately. Currently, in Foxglove map panel for example, it colors the icon based on the message. If the position fails, and it stops publishing, their UI will just show the last message received, but it just looks like the vehicle has stopped moving. When you publish status known, it colors the icon red. I do not agree with your assertion there; that's not how it's used in all cases. If you think the above use case is abusing the message, then perhaps you can refer to some documentation on usage that would disagree with Foxglove's implementation?

If the message has not started to be published or stops being published it is not reliable and should not be displayed. In the later case, it is no longer valid. What decay rate does the Foxglove map panel have for that type of message before it removes it??

NavSatFix is a message from a simpler time for GPS devices where accuracy was really 10s of meters - suitable for navigation by boats on water! Hence the NMEA standard.

If I was asked to vote to add that new status item, I would vote no - it will break code for little gain IMHO.

In my projects, I don't use NavSatFix, the publishing of it is really being added so that the projects can visualise the lat,lon,alt in high precision with existing tools.

@danthony06
Copy link

If we're successfully communicating with the GPS, I expect to see ROS messages, even if everything is invalid. Otherwise, I don't know if the device has malfunctioned, lost communications, or my node is simply misconfigured.

This is also consistent with other sensors. For example, laser rangefinders typically still publish NaNs if they can't measure a target's distance, lidars will still publish pointclouds with all NaNs, and IMUs will publish even if they're unable to measure state in the devices I've used.

Copy link

@evenator evenator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No objections from me.

@hortovanyi
Copy link

If we're successfully communicating with the GPS, I expect to see ROS messages, even if everything is invalid. Otherwise, I don't know if the device has malfunctioned, lost communications, or my node is simply misconfigured.

This is also consistent with other sensors. For example, laser rangefinders typically still publish NaNs if they can't measure a target's distance, lidars will still publish pointclouds with all NaNs, and IMUs will publish even if they're unable to measure state in the devices I've used.

I've written a few drivers now for lidars, computer vision cameras, can bus, IMUs etc - the most complicated are the GPS drivers - I don't agree with the above :) ... check the log files

@Ryanf55
Copy link
Contributor Author

Ryanf55 commented Apr 19, 2023

@tfoote If I remove the unknown status, and just have uint16 SERVICE_UNKNOWN = 0 defined for this PR, can this make Iron? I can't seem to get agreement on anything else.

@clalancette
Copy link
Contributor

@tfoote If I remove the unknown status, and just have uint16 SERVICE_UNKNOWN = 0 defined for this PR, can this make Iron? I can't seem to get agreement on anything else.

Unfortunately, no. The freeze was earlier this week. But if we get agreement on this, we can merge it into Rolling and have it ready for J-Turtle.

@Ryanf55 Ryanf55 changed the title [IRON] Fix unitialized values in NavSatFix and add missing UNKNOWN [J-Turtle] Fix uninitialized values in NavSatFix and add missing NavSatStatus UNKNOWN Apr 19, 2023
@StepTurtle
Copy link

Hey, I agree with your opinion about adding additional field for STATUS_RTK, STATUS_DGPS. Also, if it is possible, i think more information will be good. For example, my custom GPS message have these fields:

GPS_POSITION_TYPE_NO_SOLUTION = 0
GPS_POSITION_TYPE_UNKNOWN = 1
GPS_POSITION_TYPE_SINGLE = 2
GPS_POSITION_TYPE_PSRDIFF = 3
GPS_POSITION_TYPE_SBAS = 4
GPS_POSITION_TYPE_OMNISTAR = 5
GPS_POSITION_TYPE_RTK_FLOAT = 6
GPS_POSITION_TYPE_RTK_INT = 7
GPS_POSITION_TYPE_PPP_FLOAT = 8
GPS_POSITION_TYPE_PPP_INT = 9
GPS_POSITION_TYPE_FIXED = 10

So, Are you thinking adding new fields or just add STUTUS_RTK?

@danthony06
Copy link

For gps_common, we are wrapping the gpsd client, so we mostly use the statuses they define here: https://gitlab.com/gpsd/gpsd/-/blob/master/include/gps.h#L193, which I think is pretty close to what you are proposing. I would not mention vendor specific solutions, like what I assume the _OMNISTAR member is, just because there's so many possible vendors. Instead, I would replace it with something more generic.

@Ryanf55
Copy link
Contributor Author

Ryanf55 commented Jun 20, 2023

For gps_common, we are wrapping the gpsd client, so we mostly use the statuses they define here: https://gitlab.com/gpsd/gpsd/-/blob/master/include/gps.h#L193, which I think is pretty close to what you are proposing. I would not mention vendor specific solutions, like what I assume the _OMNISTAR member is, just because there's so many possible vendors. Instead, I would replace it with something more generic.

Since ROS messages and API's already try to align with Linux conventions, this seems like a great idea. Agree on not adding vendor-specific messages. I don't see why a ROS consumers of GPS data would care which vendor their GPS sensor is using for corrections; I haven't seen other messages in this repo that have vendor specifics.

@danthony06
Copy link

I'd suggest adding a DGPS definition, because that's relatively common. Is PSRDIFF specific to Novatel receivers? I've only seen it used there, but I'm not sure if it's something proprietary to them.

@StepTurtle
Copy link

To the best of my knowledge, PSRDIFF is not specific to Novatel receivers and can be used for all GPS receivers. Additionally, I highly support the idea of adding a DGPS field.

Moreover, I believe it would be beneficial to have more detailed fields for corrections, as it often requires checking the GPS interface. To address this issue, I have started using the Rviz plugin, which displays various GPS features, including corrections. Consequently, I currently utilize a custom message to send GPS status. Therefore, having more status fields is important to me. However, if other ROS users do not share this preference, I understand and respect their perspective.

Regarding the OMNISTAR field, you are correct that it is very specific. I included all the fields from my example usage to demonstrate this to you.

Furthermore, I have examined the gpsd repository you mentioned, @danthony06, and I am considering using a similar solution. Thank you for bringing it to my attention, and I appreciate your interest in this matter.

@clalancette clalancette added the more-information-needed Further information is required label Jun 29, 2023
@peci1
Copy link
Contributor

peci1 commented Nov 14, 2023

I'd like to get this moving again.

First of all, I have to say stability of interfaces is a great part of ROS. However, if the current definition is based on a 15-year-old technical state and even such simple things as adding a few (fake) enum values takes years (it was requested for ROS 1 in 2020), it really hits hard on the usability of ROS and leads to big fragmentation where everyone implements his own message type because the available ones do not fit.

Let's first review existing ROS interfaces. Except NavSatStatus, there is gps_common/GPSStatus. It defines the following constants:

int16 STATUS_NO_FIX=-1   # Unable to fix position
int16 STATUS_FIX=0       # Normal fix
int16 STATUS_SBAS_FIX=1  # Fixed using a satellite-based augmentation system
int16 STATUS_GBAS_FIX=2  #          or a ground-based augmentation system
int16 STATUS_DGPS_FIX=18 # Fixed with DGPS
int16 STATUS_WAAS_FIX=33 # Fixed with WAAS

Don't ask me where 18 and 33 originated. I don't know and neither does git blame. WAAS is a special case of SBAS, so that can be ignored as there already is agreement that vendor-specific things should not be a part of this message.

As this is the only other widely used generic ROS interface, I would suggest to keep the values of constants in NavSatStatus compatible. There's plainly no reason to redefine them differently (except for having a nice increasing number sequence).

So my suggestion is:

# See https://gssc.esa.int/navipedia/index.php/GNSS_Augmentation for explanations
int8 STATUS_UNKNOWN = -2        # status is not yet set
int8 STATUS_NO_FIX =  -1        # unable to fix position
int8 STATUS_FIX =      0        # unaugmented fix (refered to as 'single' in RTK receivers)
int8 STATUS_SBAS_FIX = 1        # fix with satellite-based augmentation
int8 STATUS_GBAS_FIX = 2        # fix with ground-based augmentation, i.e. radio beacons near airports
# value of the following constant is chosen to correspond to the value used in gps_common/GPSStatus
int8 STATUS_DGPS_FIX=18     # fix with DGPS (pseudorange corrections, sometimes referred to as 'differential fix')
int8 STATUS_RTK_FLOAT=3     # RTK with float ambiguities
int8 STATUS_RTK_FIX=4     # RTK with fixed ambiguities
int8 STATUS_PPP_FLOAT=5     # PPP with float ambiguities
int8 STATUS_PPP_FIX=6     # PPP with fixed ambiguities
int8 STATUS_REGULATED_FIX=7     # fix using restricted or regulated channels (only if no other augmentation is used)

This should cover most of the generally used algorithms for now and close future.

Regarding statuses with dead-reckoning - I don't think they need special treatment. If the DR is short, the precision of the last valid mode will mostly hold, so the driver can keep e.g. RTK fix. If the DR is longer, the driver should downgrade to FIX and record the growing variance. But maybe I'm wrong on this one, I've never worked with a DR receiver.

Regarding diff_soln, I think that it is already covered by the RTK fix constants. I.e. there can't be a RTK float or fix solution unless differential corrections were used (AFAIK). And if the user wants to know the receiver is RTK capable but did not use the corrections, this should really go into diagnostics. This message should really only serve as the common denominator for many types of receivers. The same holds e.g. for age of corrections. Either they're fresh and the status is RTK float/fix, or they're stale and the status is only FIX without augmentation. Details about age of corrections etc. should go into vendor-specific or diagnostic messages.

ROS drivers should be dumb passthrus as much as possible— so basically, a packet of data comes in from the device -> a ROS message gets published

I always treated ROS drivers as a bit more clever pieces. Each manufacturer has different SDK with different approaches. But ROS has some standard not only regarding data in the messages, but also regarding when the messages should be published. The role of the ROS driver is to adapt the SDK to the ROS API. Sometimes it's just passthrough, sometimes it requires more sophisticated transformations.

Regarding STATUS_UNKNOWN, I think it doesn't hurt to add it, but it would have to have a clearly differentiated meaning from NO_FIX (currently, both seem very similar to me). Receivers could e.g. use it to signal that the rest of the message is invalid (for whatever reason) (but doesn't NO_FIX mean the same?). It would be great to agree that drivers should periodically publish this unknown status (or NO_FIX) as soon as they establish communication with the receiver. E.g. UNKNOWN would be the output before first fix, while NO_FIX would be the output when an already acquired fix is lost. I don't think it would be too difficult for ublox to add a simple timer to their driver that would publish these UNKNOWN/NO_FIX messages until a fix is acquired. It's actually the same problem with the already existing NO_FIX constant. Some drivers will never use it because they just don't publish when fix is unavailable. But I think that's wrong from the point of view of clients. As said earlier in this thread, it would be great if clients would not need to implement the timeouts, but the drivers did that for them.

Regarding future extensibility, it might also be good to consider moving the constant definitions to their own .msg files. That would have the great benefit that adding a new constant would not change the signature/hash of the status message, so adding new constants in the future would not be such a pain for downstream packages and inter-release interoperability. This approach can actually be used right away as NavSatStatus needs to be changed anyway to incorporate the default value (be it NO_FIX or UNKNOWN). For legacy reasons, the few constants defined in NavSatStatus could also be left there, with a comment that more constants are available in another .msg file.

@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/future-of-ros-2-gps-support/33297/37

@Ryanf55
Copy link
Contributor Author

Ryanf55 commented Feb 28, 2024

As said earlier in this thread, it would be great if clients would not need to implement the timeouts, but the drivers did that for them.

I like your statement: As said earlier in this thread, it would be great if clients would not need to implement the timeouts, but the drivers did that for them.

The part about great to agree that drivers should periodically publish this unknown status (or NO_FIX) as soon as they establish communication with the receiver was exactly my intent. Or, you start a ROS driver expecting serial data at a certain baud rate, but your GPS is using the wrong baud, so the driver needs to report that it has no clue what the fix state is.

Publishing NO_FIX makes it clear to consuming libraries that the ROS driver is up and running, but that it does not have connection to the GPS sensor device.

Would you be ok if I took the minimum changes in the PR to fix the known issues, and then you do a follow up PR for adding the new constants as you propose above?

Now is the time to do message work for J-turtle.

@peci1
Copy link
Contributor

peci1 commented Feb 28, 2024

If that was aimed at me, then sure, go ahead ;)

@Ryanf55
Copy link
Contributor Author

Ryanf55 commented Mar 5, 2024

If that was aimed at me, then sure, go ahead ;)

Yup. If you agree with the current PR, then can you approve?

I would like to document this context somewhere, but the message definition does not seem like the right place. Does anyone know where a good spot to document advanced usage and intent of messages is?

@hortovanyi
Copy link

Here are the standard fix code for NMEA that I believe NavSatFix is based on - this change doesnt make sense to me. However the UBX codes we use are different from the old NMEA

https://www.sparkfun.com/datasheets/GPS/NMEA%20Reference%20Manual-Rev2.1-Dec07.pdf

Table 1-4 Position Fix Indicator
Value Description
0 Fix not available or invalid
1 GPS SPS Mode, fix valid
2 Differential GPS, SPS Mode, fix valid
3-5 Not supported
6 Dead Reckoning Mode, fix valid

sensor_msgs/msg/NavSatStatus.msg Outdated Show resolved Hide resolved
Copy link
Contributor

@tfoote tfoote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest that we not complicate this by doing the NaN change now too. There was a bunch of discussion in June last year.

And there's a concrete proposal to extend the enumeration of fix types here: #220 (comment) from @peci1

ANd a thread from the end of the year proposing a new message. https://discourse.ros.org/t/future-of-ros-2-gps-support/33297/54 that would seem like a good idea too. @SteveMacenski

But as I haven't seen progress on that I think it would make sense to do at least this iteration for now while the next generation is being designed.

sensor_msgs/msg/NavSatStatus.msg Outdated Show resolved Hide resolved
@@ -4,12 +4,13 @@
# type and the last time differential corrections were received. A
# fix is valid when status >= STATUS_FIX.

int8 STATUS_UNKNOWN = -2 # status is not yet set
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding in the NaN logic is reasonable, but I think that should be a sanity check of the data, more than the canonical way to know what the status is. Checking all field values for NaN can be very expensive versus being able to quickly check an enumeration and stop processing data early.

@SteveMacenski
Copy link
Contributor

SteveMacenski commented Mar 23, 2024

@tfoote I would just like to mention that that next generation isn't a fast moving freight train to the future and very well might not be something we ultimately spearhead if the right partners and collaborators don't assemble. I would not take the discussions in that thread as a indicator that this is the immediate priority - because frankly it doesn't seem to be from anyone involved thus far (but very hopeful that we'll get started with some parts of it sooner than later).

Its looking increasingly like it will be scaled back to improvements over the handling of GPS data with new frameworks (i.e. replace navsattransform; use 'new' & standard GPS processing libraries) within ROS with new coordinate systems (optional UTM, ECEF, etc) rather than addressing the core message interface. Core interface message changes for RTK and other subtle details I think should be spearheaded by more active users of that data and experience deploying some of the more advanced systems available. Just like the vision_msgs or radar_msgs, I'm not completely blind to activities happening in those spaces, but I view my role in those efforts as:

  • A activation function for the discussion to create the conditions for an open discussion about needs and recruit those with vested interests and expertise to join in
  • When things stall out, prodding folks to get it over the finish line
  • Rectify when competing interests exist a solution that seems rational and meeting the intent of the most applications / user's needs
  • being available to iterate over the eventual issues over the next 1-2 years that inevitably come up to handle the edge cases we missed

I'd welcome someone like @Ryanf55 or others to come and propose what that new future looks like on the interfaces side of things for replacing or augmenting NavSatFix and NavSatStatus in the modern age of GNSS / RTK solutions. I wasn't / aren't doing it because I have some unique insight, but rather that I see a problem. I'm more than happy to pass that flag onto someone else with more energy or immediate need to wrangle the cats, since its frankly not the top priority or top 5 priorities for me right now -- at least until the broader GPS handling/coordinate system work is at a testing prototype level. That thread took an unexpected turn to talk about interfaces, that wasn't what I proposed in the initial thread prompt (if you read my initial message) and I didn't commit that group or myself to the new interface design work on any prioritized timeline.

Tl;dr: I wouldn't block 'good' here in expectation of 'great' coming along any day now. Some effort that I'm related to might come along before long that touches alot of GPS capabilities, but interface design isn't currently in that imminent scope. I encourage others with a vested interest and expertise to take that on and I'll be an early adopter of it. A group of 3-4 expert end users and 3-4 GNSS/INS manufacturers with ROS knowledge could get alot done in a few calls over a quarter, I think. Happy to help support assembling that group if someone wants to take the lead on it and bring it over the line

Co-authored-by: Martin Pecka <peci1@seznam.cz>
Signed-off-by: Tully Foote <tully.foote@gmail.com>
@tfoote
Copy link
Contributor

tfoote commented Mar 25, 2024

+1 @SteveMacenski Yes I'd definitely like to land this incremental change. There was the one worry from @peci1 about the bitfield logic that I wanted to resolve. I've accepted his documentation suggestion to make that less of a worry.

As we're approaching the cutoff for Jazzy feature freeze I'd like to land this as is without increasing scope. With the current scope of only adding enumerations it's backwards compatilble.

Does anyone have any objections to doing this minimal change as proposed now? @Ryanf55 @peci1 @dagar

Copy link
Contributor

@peci1 peci1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Tully, I'm happy with the PR now!

@Ryanf55
Copy link
Contributor Author

Ryanf55 commented Mar 25, 2024

+1 @SteveMacenski Yes I'd definitely like to land this incremental change. There was the one worry from @peci1 about the bitfield logic that I wanted to resolve. I've accepted his documentation suggestion to make that less of a worry.

As we're approaching the cutoff for Jazzy feature freeze I'd like to land this as is without increasing scope. With the current scope of only adding enumerations it's backwards compatilble.

Does anyone have any objections to doing this minimal change as proposed now? @Ryanf55 @peci1 @dagar

Approved!

@Ryanf55
Copy link
Contributor Author

Ryanf55 commented Mar 25, 2024

I'd welcome someone like @Ryanf55 or others to come and propose what that new future looks like on the interfaces side of things for replacing or augmenting NavSatFix and NavSatStatus in the modern age of GNSS / RTK solutions. I wasn't / aren't doing it because I have some unique insight, but rather that I see a problem. I'm more than happy to pass that flag onto someone else with more energy or immediate need to wrangle the cats, since its frankly not the top priority or top 5 priorities for me right now -- at least until the broader GPS handling/coordinate system work is at a testing prototype level. That thread took an unexpected turn to talk about interfaces, that wasn't what I proposed in the initial thread prompt (if you read my initial message) and I didn't commit that group or myself to the new interface design work on any prioritized timeline.

Tl;dr: I wouldn't block 'good' here in expectation of 'great' coming along any day now. Some effort that I'm related to might come along before long that touches alot of GPS capabilities, but interface design isn't currently in that imminent scope. I encourage others with a vested interest and expertise to take that on and I'll be an early adopter of it. A group of 3-4 expert end users and 3-4 GNSS/INS manufacturers with ROS knowledge could get alot done in a few calls over a quarter, I think. Happy to help support assembling that group if someone wants to take the lead on it and bring it over the line

I'm happy to take this up in May. MicroStrain is a GNSS-INS manufacturer who understands ROS and also is very good at API design; I think we can leverage their expertise in designing messages for GNSS-INS. I also have a good relationship with Trimble who make the PX-1. It's pretty clear from the discussions here that NavSatFix is not appropriate for many use cases with GNSS-INS, and there is also a need for vendor-specific messages anyways.

Is anyone interested in able to plan on a small working group in May to focus on interface design for GNSS-INS to try to cover some of the items here:

  • Support 3D orientation
  • dual-antenna systems
  • Have clearer enums for the different types of corrections
  • Clear states for when the device is not connected vs has no GPS fix
  • Support RTK, RTX, and other types of correction services
  • Guidelines for vendor-specific messing and perhaps some examples with existing UBX driver
  • Add dedicated 3D velocity data
  • Clear ways to represent accuracy of the data

I don't consider myself an expert, but I do have time and access to multiple vendor's hardware to test on

@tfoote tfoote requested a review from dagar March 25, 2024 17:38
@tfoote
Copy link
Contributor

tfoote commented Apr 2, 2024

Thanks for all the effort to iterate on this change. It's small but will hopefully be helpful. I'm going to merge it as is and thanks @Ryanf55 for planning to kick off the planning for the next cycle after the release.

@tfoote tfoote merged commit d407bd0 into ros2:rolling Apr 2, 2024
2 of 3 checks passed
@Ryanf55 Ryanf55 deleted the fix-unitialized-values branch April 2, 2024 21:17
@clalancette
Copy link
Contributor

I didn't see a CI run for this change. Given that it is pretty minimal, I don't expect a lot of fallout, but that long line might run afoul of our linters. And it is just our best practice to run CI on all changes.

Here is some CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

tfoote added a commit that referenced this pull request Apr 10, 2024
…atStatus UNKNOWN (#220)

* Fix unitialized values in NavSatFix and add missing UNKNOWN
* Fixes #196
* Fix default initialization instead of constants
* Define SERVICE_UNKNOWN

Signed-off-by: Ryan Friedman <ryanfriedman5410+github@gmail.com>
Signed-off-by: Tully Foote <tully.foote@gmail.com>
Co-authored-by: Tully Foote <tully.foote@gmail.com>
Co-authored-by: Martin Pecka <peci1@seznam.cz>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
more-information-needed Further information is required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

status field in NavSatStatus message has bad default value